Remove attrs dependency to improve import times#161
Conversation
meshy
left a comment
meshy
left a comment
There was a problem hiding this comment.
Heya! :)
Thanks for this!
I'm on board with the idea of removing attrs, but I'm not sure about the import-time being a good justification for it. Instead I think the main reason I'm keen to get this in is because it will make the work of integrating into Django easier later, if that ever comes to fruition.
Can I please ask you to remove references to timing from the reasoning for this change (including in the commit message), and focus on that other benefit instead?
Thank you!
| - Exceptions raised through `transaction_if_not_already` | ||
| when it was called from inside an existing transaction | ||
| will no longer invalidate the outer transaction's context. | ||
| - Remove `attrs` dependency to improve import time. |
There was a problem hiding this comment.
Issue: I'd like to change the wording here to remove the reasoning. I think it's sufficient to say Removed `attrs` dependency.
| database: str | ||
|
|
There was a problem hiding this comment.
As you noted, these annotations aren't useful any more. Can I please ask you to remove them?
It removes the immutability of the exceptions, but as the exceptions are intended for dev and not to be handled, this trade-off is worthwhile.
RealOrangeOne
force-pushed
the
remove-attrs
branch
from
c7962dc to
589ed3b
Compare
|
Thank you! ❤️ |
2 participants
This shaves around 10ms (~18%) off the import time.
It removes the immutability of the exceptions, but as the exceptions are intended for dev and not to be handled, this trade-off is worthwhile.
I've left the attribute types in place, on the off chance they're useful. It does duplicate the types for potentially little/no gain so also happy to remove.